Skip to content

handle more nullable types#1984

Merged
kodiakhq[bot] merged 4 commits intomainfrom
mikeshi/handle-more-nullable-types
Apr 2, 2026
Merged

handle more nullable types#1984
kodiakhq[bot] merged 4 commits intomainfrom
mikeshi/handle-more-nullable-types

Conversation

@MikeShi42
Copy link
Copy Markdown
Contributor

@MikeShi42 MikeShi42 commented Mar 25, 2026

Summary

Fix ClickHouse query error when expanding log rows with Nullable(DateTime64) columns (and other Nullable types).

  • The convertCHDataTypeToJSType function didn't generically unwrap Nullable(...) types, so Nullable(DateTime64(...)) fell through to the default string comparison instead of using parseDateTime64BestEffort()
  • Added general Nullable(...) recursive unwrapping (matching the existing LowCardinality(...) pattern)
  • Hoisted null value handling above the type switch in processRowToWhereClause so all column types (Date, Array, Map, etc.) correctly emit isNull() for null values

Screenshots or video

N/A — no UI changes.

How to test locally or on Vercel

  1. Set up a ClickHouse table with a Nullable(DateTime64) column and ingest some rows (including rows with null values in that column).
  2. Open the log explorer and expand a row that has a Nullable(DateTime64) column.
  3. Verify that clicking into the row no longer returns a 400 error.
  4. Verify that clicking into a row where the Nullable(DateTime64) column is null correctly filters using isNull().

References

  • Related PRs:

📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

@MikeShi42 MikeShi42 requested review from a team and wrn14897 and removed request for a team March 25, 2026 06:47
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 25, 2026

🦋 Changeset detected

Latest commit: 82edb64

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Patch
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 25, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Apr 2, 2026 6:43am

Request Review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2026

PR Review

✅ No critical issues found.

The two fixes are correct and follow existing patterns:

  • Nullable( unwrapping in convertCHDataTypeToJSType mirrors the existing LowCardinality( pattern with correct slice offsets (9 chars)
  • Null check hoisted above the switch is a clean fix that correctly unifies handling across all types
  • Tests added for both changes; coverage looks adequate

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2026

E2E Test Results

All tests passed • 126 passed • 3 skipped • 1043s

Status Count
✅ Passed 126
❌ Failed 0
⚠️ Flaky 2
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@MikeShi42 MikeShi42 changed the title Mikeshi/handle more nullable types handle more nullable types Mar 25, 2026
@MikeShi42
Copy link
Copy Markdown
Contributor Author

@copilot can you pls fix lint errors

Copy link
Copy Markdown

Copilot AI commented Mar 25, 2026

@copilot can you pls fix lint errors

Fixed in 877d593 — sorted named imports alphabetically and corrected prettier formatting (double quotes for strings containing single quotes, and .toBe() call placement).

@MikeShi42
Copy link
Copy Markdown
Contributor Author

okay @claude maybe you can help me fix this lint issue...

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2026

Claude finished @MikeShi42's task in 57s —— View job


Fixed lint issues

  • Read relevant files
  • Fix dead code in useRowWhere.tsx — removed unreachable value == null || (null already handled at line 58) and updated stale comment
  • Remove redundant explicit Nullable(Int/UInt/Float/String) checks in clickhouse/index.ts — superseded by the generic Nullable() unwrapper
  • Committed and pushed (ede119c5)

@MikeShi42 MikeShi42 force-pushed the mikeshi/handle-more-nullable-types branch from 9ab50bc to 11ee222 Compare March 26, 2026 04:52

**Tools handle formatting and linting automatically** via pre-commit hooks.
Focus on implementation; don't manually format code.
**After finishing all code edits**, run `yarn lint:fix` to auto-fix formatting
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after losing the war with agents vs linter, I'm telling agents to always lint - I'm not sure why we did not encourage this anyways? @teeohhem maybe you can lmk

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do document the pre-commit hook:

hyperdx/AGENTS.md

Lines 160 to 163 in dea1b66

**Pre-commit hooks must pass before committing.** Do not use `--no-verify` to
skip hooks. If the pre-commit hook fails (e.g. due to husky not being set up in
a worktree), run `npx lint-staged` manually before committing to ensure lint and
formatting checks pass. Fix any issues before creating the commit.

In theory, the agent should ensure the code is formatted before pushing a new commit.

@kodiakhq kodiakhq bot merged commit 48a8d32 into main Apr 2, 2026
15 of 16 checks passed
@kodiakhq kodiakhq bot deleted the mikeshi/handle-more-nullable-types branch April 2, 2026 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants